Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Merged by Bors] - Extract Resources into their own dedicated storage #4809

Closed
wants to merge 40 commits into from

Conversation

james7132
Copy link
Member

@james7132 james7132 commented May 20, 2022

Objective

At least partially addresses #6282.

Resources are currently stored as a dedicated Resource archetype (ID 1). This allows for easy code reusability, but unnecessarily adds 72 bytes (on 64-bit systems) to the struct that is only used for that one archetype. It also requires several fields to be pub(crate) which isn't ideal.

This should also remove one sparse-set lookup from fetching, inserting, and removing resources from a World.

Solution

  • Add Resources parallel to Tables and SparseSets and extract the functionality used by Archetype in it.
  • Remove unique_components from Archetype
  • Remove the pub(crate) on Archetype::components.
  • Remove ArchetypeId::RESOURCE
  • Remove Archetypes::resource and Archetypes::resource_mut

Changelog

Added: Resources type to store resources.
Added: Storages::resource
Removed: ArchetypeId::RESOURCE
Removed: Archetypes::resource and Archetypes::resources
Removed: Archetype::unique_components and Archetypes::unique_components_mut

Migration Guide

Resources have been moved to Resources under Storages in World. All code dependent on Archetype::unique_components(_mut) should access it via world.storages().resources() instead.

All APIs accessing the raw data of individual resources (mutable and read-only) have been removed as these APIs allowed for unsound unsafe code. All usages of these APIs should be changed to use World::{get, insert, remove}_resource.

@james7132 james7132 added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change labels May 20, 2022
@alice-i-cecile alice-i-cecile requested a review from BoxyUwU May 30, 2022 17:39
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very happy with the direction here; this is much cleaner.

A couple of small changes needed.

  1. Finish the migration guide.
  2. Add doc strings to Resources.

Optionally:

  1. Refactor the common snippet noted above into a function.
  2. Add doc strings to the methods on Resources.

crates/bevy_ecs/src/world/world_cell.rs Show resolved Hide resolved
crates/bevy_ecs/src/storage/resource.rs Outdated Show resolved Hide resolved
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bevyengine/ecs-team This one is easy; reviews?

@alice-i-cecile
Copy link
Member

@james7132 can you fix this up when you get the chance? I think the fixes should be straightforward, and I'd like to unblock #4955.

@james7132 james7132 requested a review from alice-i-cecile June 7, 2022 09:10
@james7132 james7132 force-pushed the no-unique-components branch from d8758f8 to 4918e10 Compare June 7, 2022 09:11
@james7132 james7132 added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jun 11, 2022
@alice-i-cecile
Copy link
Member

Ping @BoxyUwU for attempt 5 to remember to review this 😂

Copy link
Member

@BoxyUwU BoxyUwU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5th times the charm I suppose

crates/bevy_ecs/src/storage/resource.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/storage/resource.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/storage/resource.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/storage/resource.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/storage/resource.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/storage/resource.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/mod.rs Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Jun 12, 2022
@james7132 james7132 added C-Bug An unexpected or incorrect behavior P-Unsound A bug that results in undefined compiler behavior labels Oct 18, 2022
@alice-i-cecile
Copy link
Member

I think we can do those in follow-up PRs. It's useful, but not essential.

@james7132 james7132 requested a review from BoxyUwU October 18, 2022 03:16
Copy link
Member

@BoxyUwU BoxyUwU left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly this is still kinda sketchy around Send/Sync but I am feeling like we ought to just merge this and move on because trying to get bevy to be sound with World: Send + Sync seems (to me) like a losing battle. Since this PR doesnt actually make anything worse we should just merge this and make World: !Send + !Sync hold later.

@alice-i-cecile
Copy link
Member

This is ready: I agree with Boxy's assessment and think this makes reasoning about the internals of resources much nicer. Small perf improvements are a nice bonus.

bors r+

bors bot pushed a commit that referenced this pull request Oct 24, 2022
# Objective
At least partially addresses #6282.

Resources are currently stored as a dedicated Resource archetype (ID 1). This allows for easy code reusability, but unnecessarily adds 72 bytes (on 64-bit systems) to the struct that is only used for that one archetype. It also requires several fields to be `pub(crate)` which isn't ideal.

This should also remove one sparse-set lookup from fetching, inserting, and removing resources from a `World`.

## Solution

- Add `Resources` parallel to `Tables` and `SparseSets` and extract the functionality used by `Archetype` in it.
- Remove `unique_components` from `Archetype`
- Remove the `pub(crate)` on `Archetype::components`.
- Remove `ArchetypeId::RESOURCE`
- Remove `Archetypes::resource` and `Archetypes::resource_mut`

---

## Changelog
Added: `Resources` type to store resources.
Added: `Storages::resource`
Removed: `ArchetypeId::RESOURCE`
Removed: `Archetypes::resource` and `Archetypes::resources`
Removed: `Archetype::unique_components` and `Archetypes::unique_components_mut`

## Migration Guide
Resources have been moved to `Resources` under `Storages` in `World`. All code dependent on `Archetype::unique_components(_mut)` should access it via `world.storages().resources()` instead.

All APIs accessing the raw data of individual resources (mutable *and* read-only) have been removed as these APIs allowed for unsound unsafe code. All usages of these APIs should be changed to use `World::{get, insert, remove}_resource`.
@bors bors bot changed the title Extract Resources into their own dedicated storage [Merged by Bors] - Extract Resources into their own dedicated storage Oct 24, 2022
@bors bors bot closed this Oct 24, 2022
james7132 added a commit to james7132/bevy that referenced this pull request Oct 28, 2022
At least partially addresses bevyengine#6282.

Resources are currently stored as a dedicated Resource archetype (ID 1). This allows for easy code reusability, but unnecessarily adds 72 bytes (on 64-bit systems) to the struct that is only used for that one archetype. It also requires several fields to be `pub(crate)` which isn't ideal.

This should also remove one sparse-set lookup from fetching, inserting, and removing resources from a `World`.

- Add `Resources` parallel to `Tables` and `SparseSets` and extract the functionality used by `Archetype` in it.
- Remove `unique_components` from `Archetype`
- Remove the `pub(crate)` on `Archetype::components`.
- Remove `ArchetypeId::RESOURCE`
- Remove `Archetypes::resource` and `Archetypes::resource_mut`

---

Added: `Resources` type to store resources.
Added: `Storages::resource`
Removed: `ArchetypeId::RESOURCE`
Removed: `Archetypes::resource` and `Archetypes::resources`
Removed: `Archetype::unique_components` and `Archetypes::unique_components_mut`

Resources have been moved to `Resources` under `Storages` in `World`. All code dependent on `Archetype::unique_components(_mut)` should access it via `world.storages().resources()` instead.

All APIs accessing the raw data of individual resources (mutable *and* read-only) have been removed as these APIs allowed for unsound unsafe code. All usages of these APIs should be changed to use `World::{get, insert, remove}_resource`.
bors bot pushed a commit that referenced this pull request Nov 15, 2022
…4927)

# Objective
Archetype is a deceptively large type in memory. It stores metadata about which components are in which storage in multiple locations, which is only used when creating new Archetypes while moving entities.

## Solution
Remove the redundant `Box<[ComponentId]>`s and iterate over the sparse set of component metadata instead. Reduces Archetype's size by 4 usizes (32 bytes on 64-bit systems), as well as the additional allocations for holding these slices.

It'd seem like there's a downside that the origin archetype has it's component metadata iterated over twice when creating a new archetype, but this change also removes the extra `Vec<ArchetypeComponentId>` allocations when creating a new archetype which may amortize out to a net gain here. This change likely negatively impacts creating new archetypes with a large number of components, but that's a cost mitigated by the fact that these archetypal relationships are cached in Edges and is incurred only once for each edge created.

## Additional Context
There are several other in-flight PRs that shrink Archetype:

 - #4800 merges the entities and rows Vecs together (shaves off 24 bytes per archetype) 
 - #4809 removes unique_components and moves it to it's own dedicated storage (shaves off 72 bytes per archetype)

---

## Changelog
Changed: `Archetype::table_components` and `Archetype::sparse_set_components` return iterators instead of slices. `Archetype::new` requires iterators instead of parallel slices/vecs.

## Migration Guide
Do I still need to do this? I really hope people were not relying on the public facing APIs changed here.
bors bot pushed a commit that referenced this pull request Nov 15, 2022
…4927)

# Objective
Archetype is a deceptively large type in memory. It stores metadata about which components are in which storage in multiple locations, which is only used when creating new Archetypes while moving entities.

## Solution
Remove the redundant `Box<[ComponentId]>`s and iterate over the sparse set of component metadata instead. Reduces Archetype's size by 4 usizes (32 bytes on 64-bit systems), as well as the additional allocations for holding these slices.

It'd seem like there's a downside that the origin archetype has it's component metadata iterated over twice when creating a new archetype, but this change also removes the extra `Vec<ArchetypeComponentId>` allocations when creating a new archetype which may amortize out to a net gain here. This change likely negatively impacts creating new archetypes with a large number of components, but that's a cost mitigated by the fact that these archetypal relationships are cached in Edges and is incurred only once for each edge created.

## Additional Context
There are several other in-flight PRs that shrink Archetype:

 - #4800 merges the entities and rows Vecs together (shaves off 24 bytes per archetype) 
 - #4809 removes unique_components and moves it to it's own dedicated storage (shaves off 72 bytes per archetype)

---

## Changelog
Changed: `Archetype::table_components` and `Archetype::sparse_set_components` return iterators instead of slices. `Archetype::new` requires iterators instead of parallel slices/vecs.

## Migration Guide
Do I still need to do this? I really hope people were not relying on the public facing APIs changed here.
bors bot pushed a commit that referenced this pull request Nov 15, 2022
# Objective
Make core types in ECS smaller. The column sparse set in Tables is never updated after creation.

## Solution
Create `ImmutableSparseSet` which removes the capacity fields in the backing vec's and the APIs for inserting or removing elements. Drops the size of the sparse set by 3 usizes (24 bytes on 64-bit systems)

## Followup
~~After #4809, Archetype's component SparseSet should be replaced with it.~~ This has been done.

---

## Changelog
Removed: `Table::component_capacity`

## Migration Guide
`Table::component_capacity()` has been removed as Tables do not support adding/removing columns after construction.

Co-authored-by: Carter Anderson <[email protected]>
taiyoungjang pushed a commit to taiyoungjang/bevy that referenced this pull request Dec 15, 2022
…evyengine#4927)

# Objective
Archetype is a deceptively large type in memory. It stores metadata about which components are in which storage in multiple locations, which is only used when creating new Archetypes while moving entities.

## Solution
Remove the redundant `Box<[ComponentId]>`s and iterate over the sparse set of component metadata instead. Reduces Archetype's size by 4 usizes (32 bytes on 64-bit systems), as well as the additional allocations for holding these slices.

It'd seem like there's a downside that the origin archetype has it's component metadata iterated over twice when creating a new archetype, but this change also removes the extra `Vec<ArchetypeComponentId>` allocations when creating a new archetype which may amortize out to a net gain here. This change likely negatively impacts creating new archetypes with a large number of components, but that's a cost mitigated by the fact that these archetypal relationships are cached in Edges and is incurred only once for each edge created.

## Additional Context
There are several other in-flight PRs that shrink Archetype:

 - bevyengine#4800 merges the entities and rows Vecs together (shaves off 24 bytes per archetype) 
 - bevyengine#4809 removes unique_components and moves it to it's own dedicated storage (shaves off 72 bytes per archetype)

---

## Changelog
Changed: `Archetype::table_components` and `Archetype::sparse_set_components` return iterators instead of slices. `Archetype::new` requires iterators instead of parallel slices/vecs.

## Migration Guide
Do I still need to do this? I really hope people were not relying on the public facing APIs changed here.
taiyoungjang pushed a commit to taiyoungjang/bevy that referenced this pull request Dec 15, 2022
# Objective
Make core types in ECS smaller. The column sparse set in Tables is never updated after creation.

## Solution
Create `ImmutableSparseSet` which removes the capacity fields in the backing vec's and the APIs for inserting or removing elements. Drops the size of the sparse set by 3 usizes (24 bytes on 64-bit systems)

## Followup
~~After bevyengine#4809, Archetype's component SparseSet should be replaced with it.~~ This has been done.

---

## Changelog
Removed: `Table::component_capacity`

## Migration Guide
`Table::component_capacity()` has been removed as Tables do not support adding/removing columns after construction.

Co-authored-by: Carter Anderson <[email protected]>
Pietrek14 pushed a commit to Pietrek14/bevy that referenced this pull request Dec 17, 2022
# Objective
At least partially addresses bevyengine#6282.

Resources are currently stored as a dedicated Resource archetype (ID 1). This allows for easy code reusability, but unnecessarily adds 72 bytes (on 64-bit systems) to the struct that is only used for that one archetype. It also requires several fields to be `pub(crate)` which isn't ideal.

This should also remove one sparse-set lookup from fetching, inserting, and removing resources from a `World`.

## Solution

- Add `Resources` parallel to `Tables` and `SparseSets` and extract the functionality used by `Archetype` in it.
- Remove `unique_components` from `Archetype`
- Remove the `pub(crate)` on `Archetype::components`.
- Remove `ArchetypeId::RESOURCE`
- Remove `Archetypes::resource` and `Archetypes::resource_mut`

---

## Changelog
Added: `Resources` type to store resources.
Added: `Storages::resource`
Removed: `ArchetypeId::RESOURCE`
Removed: `Archetypes::resource` and `Archetypes::resources`
Removed: `Archetype::unique_components` and `Archetypes::unique_components_mut`

## Migration Guide
Resources have been moved to `Resources` under `Storages` in `World`. All code dependent on `Archetype::unique_components(_mut)` should access it via `world.storages().resources()` instead.

All APIs accessing the raw data of individual resources (mutable *and* read-only) have been removed as these APIs allowed for unsound unsafe code. All usages of these APIs should be changed to use `World::{get, insert, remove}_resource`.
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
…evyengine#4927)

# Objective
Archetype is a deceptively large type in memory. It stores metadata about which components are in which storage in multiple locations, which is only used when creating new Archetypes while moving entities.

## Solution
Remove the redundant `Box<[ComponentId]>`s and iterate over the sparse set of component metadata instead. Reduces Archetype's size by 4 usizes (32 bytes on 64-bit systems), as well as the additional allocations for holding these slices.

It'd seem like there's a downside that the origin archetype has it's component metadata iterated over twice when creating a new archetype, but this change also removes the extra `Vec<ArchetypeComponentId>` allocations when creating a new archetype which may amortize out to a net gain here. This change likely negatively impacts creating new archetypes with a large number of components, but that's a cost mitigated by the fact that these archetypal relationships are cached in Edges and is incurred only once for each edge created.

## Additional Context
There are several other in-flight PRs that shrink Archetype:

 - bevyengine#4800 merges the entities and rows Vecs together (shaves off 24 bytes per archetype) 
 - bevyengine#4809 removes unique_components and moves it to it's own dedicated storage (shaves off 72 bytes per archetype)

---

## Changelog
Changed: `Archetype::table_components` and `Archetype::sparse_set_components` return iterators instead of slices. `Archetype::new` requires iterators instead of parallel slices/vecs.

## Migration Guide
Do I still need to do this? I really hope people were not relying on the public facing APIs changed here.
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
# Objective
Make core types in ECS smaller. The column sparse set in Tables is never updated after creation.

## Solution
Create `ImmutableSparseSet` which removes the capacity fields in the backing vec's and the APIs for inserting or removing elements. Drops the size of the sparse set by 3 usizes (24 bytes on 64-bit systems)

## Followup
~~After bevyengine#4809, Archetype's component SparseSet should be replaced with it.~~ This has been done.

---

## Changelog
Removed: `Table::component_capacity`

## Migration Guide
`Table::component_capacity()` has been removed as Tables do not support adding/removing columns after construction.

Co-authored-by: Carter Anderson <[email protected]>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective
At least partially addresses bevyengine#6282.

Resources are currently stored as a dedicated Resource archetype (ID 1). This allows for easy code reusability, but unnecessarily adds 72 bytes (on 64-bit systems) to the struct that is only used for that one archetype. It also requires several fields to be `pub(crate)` which isn't ideal.

This should also remove one sparse-set lookup from fetching, inserting, and removing resources from a `World`.

## Solution

- Add `Resources` parallel to `Tables` and `SparseSets` and extract the functionality used by `Archetype` in it.
- Remove `unique_components` from `Archetype`
- Remove the `pub(crate)` on `Archetype::components`.
- Remove `ArchetypeId::RESOURCE`
- Remove `Archetypes::resource` and `Archetypes::resource_mut`

---

## Changelog
Added: `Resources` type to store resources.
Added: `Storages::resource`
Removed: `ArchetypeId::RESOURCE`
Removed: `Archetypes::resource` and `Archetypes::resources`
Removed: `Archetype::unique_components` and `Archetypes::unique_components_mut`

## Migration Guide
Resources have been moved to `Resources` under `Storages` in `World`. All code dependent on `Archetype::unique_components(_mut)` should access it via `world.storages().resources()` instead.

All APIs accessing the raw data of individual resources (mutable *and* read-only) have been removed as these APIs allowed for unsound unsafe code. All usages of these APIs should be changed to use `World::{get, insert, remove}_resource`.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…evyengine#4927)

# Objective
Archetype is a deceptively large type in memory. It stores metadata about which components are in which storage in multiple locations, which is only used when creating new Archetypes while moving entities.

## Solution
Remove the redundant `Box<[ComponentId]>`s and iterate over the sparse set of component metadata instead. Reduces Archetype's size by 4 usizes (32 bytes on 64-bit systems), as well as the additional allocations for holding these slices.

It'd seem like there's a downside that the origin archetype has it's component metadata iterated over twice when creating a new archetype, but this change also removes the extra `Vec<ArchetypeComponentId>` allocations when creating a new archetype which may amortize out to a net gain here. This change likely negatively impacts creating new archetypes with a large number of components, but that's a cost mitigated by the fact that these archetypal relationships are cached in Edges and is incurred only once for each edge created.

## Additional Context
There are several other in-flight PRs that shrink Archetype:

 - bevyengine#4800 merges the entities and rows Vecs together (shaves off 24 bytes per archetype) 
 - bevyengine#4809 removes unique_components and moves it to it's own dedicated storage (shaves off 72 bytes per archetype)

---

## Changelog
Changed: `Archetype::table_components` and `Archetype::sparse_set_components` return iterators instead of slices. `Archetype::new` requires iterators instead of parallel slices/vecs.

## Migration Guide
Do I still need to do this? I really hope people were not relying on the public facing APIs changed here.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective
Make core types in ECS smaller. The column sparse set in Tables is never updated after creation.

## Solution
Create `ImmutableSparseSet` which removes the capacity fields in the backing vec's and the APIs for inserting or removing elements. Drops the size of the sparse set by 3 usizes (24 bytes on 64-bit systems)

## Followup
~~After bevyengine#4809, Archetype's component SparseSet should be replaced with it.~~ This has been done.

---

## Changelog
Removed: `Table::component_capacity`

## Migration Guide
`Table::component_capacity()` has been removed as Tables do not support adding/removing columns after construction.

Co-authored-by: Carter Anderson <[email protected]>
@james7132 james7132 deleted the no-unique-components branch March 14, 2023 04:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide P-Unsound A bug that results in undefined compiler behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants